Conversation
This is to: 1. Fix compile error due to duplicate definition, and 2. Reflect how it is the real C++ standard library.
std::pair does not extend std::tuple.
…lass-specific inline definitions
These actually can be detected!
If an operator is declared multiple times including application code, this class would err on the conservative side and treat it as a standard library one.
When it comes to declaration, treat everything as non-compliant, including the ones that are replaceable allocation / non-allocation functions.
….expected due to changes in functional.h and utility.h
…ted due to changes in functional.h and utility.h
There was a problem hiding this comment.
Pull request overview
Adds new CodeQL rule packages and queries for MISRA C++:2023 Rule 21.6.2 and 21.6.3, including corresponding unit tests and updates to the standard-library test stubs required to model advanced/dynamic memory APIs.
Changes:
- Register new rule packages
Memory5(RULE-21-6-2) andMemory6(RULE-21-6-3) inrules.csvand add their package JSON descriptors. - Add new MISRA query implementations for detecting manual dynamic memory management and advanced memory management patterns, plus unit tests and expected outputs.
- Extend/adjust C++ standard-library test stubs (e.g.,
<memory>,<memory_resource>,<tuple>,<utility>,<cstdlib>,<stddef.h>) to support the new queries/tests.
Reviewed changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| rules.csv | Maps RULE-21-6-2/21-6-3 to new packages Memory5/Memory6. |
| rule_packages/cpp/Memory5.json | New package metadata for RULE-21-6-2. |
| rule_packages/cpp/Memory6.json | New package metadata for RULE-21-6-3. |
| cpp/common/src/codingstandards/cpp/exclusions/cpp/RuleMetadata.qll | Registers Memory5/Memory6 in the exclusion metadata wiring. |
| cpp/common/src/codingstandards/cpp/exclusions/cpp/Memory5.qll | New autogenerated exclusion metadata module for Memory5. |
| cpp/common/src/codingstandards/cpp/exclusions/cpp/Memory6.qll | New autogenerated exclusion metadata module for Memory6. |
| cpp/misra/src/rules/RULE-21-6-2/DynamicMemoryManagedManually.ql | New query for manual dynamic memory management detection. |
| cpp/misra/src/rules/RULE-21-6-3/AdvancedMemoryManagementUsed.ql | New query for advanced memory management detection. |
| cpp/misra/test/rules/RULE-21-6-2/test.cpp | New unit test cases for RULE-21-6-2. |
| cpp/misra/test/rules/RULE-21-6-2/DynamicMemoryManagedManually.qlref | Links test to the new RULE-21-6-2 query. |
| cpp/misra/test/rules/RULE-21-6-2/DynamicMemoryManagedManually.expected | Expected results for RULE-21-6-2 test. |
| cpp/misra/test/rules/RULE-21-6-3/test.cpp | New unit test cases for RULE-21-6-3. |
| cpp/misra/test/rules/RULE-21-6-3/AdvancedMemoryManagementUsed.qlref | Links test to the new RULE-21-6-3 query. |
| cpp/misra/test/rules/RULE-21-6-3/AdvancedMemoryManagementUsed.expected | Expected results for RULE-21-6-3 test. |
| cpp/misra/test/rules/RULE-8-1-1/NonTransientLambdaImplicitlyCapturesThis.expected | Updates expected output due to stdlib stub line/structure changes. |
| cpp/misra/test/rules/RULE-8-1-2/ImplicitCapturesDisallowedInNonTransientLambda.expected | Updates expected output due to stdlib stub line/structure changes. |
| cpp/common/test/includes/standard-library/utility.h | Adjusts tuple/pair stubs (incl. forward-decl of tuple). |
| cpp/common/test/includes/standard-library/tuple.h | Adds tuple stub (but currently with an include-guard issue). |
| cpp/common/test/includes/standard-library/string | Adjusts allocator stub to a forward declaration. |
| cpp/common/test/includes/standard-library/stdlib.h | Adds aligned_alloc declaration to the stub. |
| cpp/common/test/includes/standard-library/stddef.h | Adds std::byte stub type. |
| cpp/common/test/includes/standard-library/memory.h | Expands <memory> stubs (allocator, uninitialized algorithms, destroy, launder). |
| cpp/common/test/includes/standard-library/cstdlib | Exposes additional C allocation APIs into std:: in the stub. |
| cpp/common/test/includes/standard-library/memory_resource.h | Adds <memory_resource> stub declarations for PMR types. |
| cpp/common/test/includes/standard-library/memory_resource | Wrapper include for the new <memory_resource.h> stub. |
| cpp/common/src/codingstandards/cpp/allocations/CustomOperatorNewDelete.qll | Refines modeling/filtering of custom operator new/delete. |
| cpp/common/src/codingstandards/cpp/UnintializedMemoryAllocation.qll | New helper library for modeling uninitialized-memory management functions. |
Comments suppressed due to low confidence (3)
cpp/misra/src/rules/RULE-21-6-3/AdvancedMemoryManagementUsed.ql:19
- The imported module name
codingstandards.cpp.UnintializedMemoryAllocationappears to be misspelled (“Unintialized”). Since this becomes part of the library API, consider renaming the module/file toUninitializedMemoryAllocationand updating imports accordingly.
import cpp
import codingstandards.cpp.misra
import codingstandards.cpp.UnintializedMemoryAllocation
import codingstandards.cpp.allocations.CustomOperatorNewDelete
rule_packages/cpp/Memory6.json:10
- Typo in package description: “uninitalized” should be “uninitialized”.
"description": "Using advanced memory management that either alters allocation and deallocation or constructs object construction on uninitalized memory may result in undefined behavior.",
cpp/misra/src/rules/RULE-21-6-2/DynamicMemoryManagedManually.ql:61
- The doc comment examples list
std::pmr::memory_resource::allocate(...)twice; the second bullet likely intended to referencedeallocate(...)(or otherwise show one allocate + one deallocate example).
* - `std::allocator<T>::allocate(std::size_t)`
* - `std::allocator<T>::dellocate(T*, std::size_t)`
* - `std::pmr::memory_resource::allocate(std::size_t, std::size_t)`
* - `std::pmr::memory_resource::allocate(std::size_t, std::size_t)`
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /** | ||
| * Provides models of functions in <memory> that deals with uninitialized memory. | ||
| */ | ||
|
|
||
| import cpp |
There was a problem hiding this comment.
This new library module/file name is misspelled (“Unintialized”). Since it’s a reusable library module, it’s worth renaming it to UninitializedMemoryAllocation.qll (and adjusting the module name/imports) to avoid locking in a typo as part of the public API.
| ] | ||
| } | ||
| ], | ||
| "title": "Using advanced memory management that either alters allocation and deallocation or constructs object construction on uninitalized memory may result in undefined behavior." |
There was a problem hiding this comment.
Typo in package title: “uninitalized” should be “uninitialized”.
This issue also appears on line 10 of the same file.
| * @description Using advanced memory management that either alters allocation and deallocation or | ||
| * constructs object construction on uninitalized memory may result in undefined | ||
| * behavior. |
There was a problem hiding this comment.
Typo in the query metadata description: “uninitalized” should be “uninitialized”.
This issue also appears on line 16 of the same file.
| /** | ||
| * A function that has namespace `std` and has name `allocate` or `deallocate`, including but not limited to: | ||
| * - `std::allocator<T>::allocate(std::size_t)` | ||
| * - `std::allocator<T>::dellocate(T*, std::size_t)` |
There was a problem hiding this comment.
Typo in doc comment example: dellocate should be deallocate.
This issue also appears on line 58 of the same file.
| * - `std::allocator<T>::dellocate(T*, std::size_t)` | |
| * - `std::allocator<T>::deallocate(T*, std::size_t)` |
| "precision": "very-high", | ||
| "severity": "error", | ||
| "short_name": "DynamicMemoryManagedManually", | ||
| "tags": [ |
There was a problem hiding this comment.
The tags list only includes the scope tag. Other rule packages typically include at least one classification tag (e.g., correctness, maintainability, reliability) in addition to scope. Consider adding an appropriate classification tag here for better categorization/filtering.
| "tags": [ | |
| "tags": [ | |
| "correctness", |
| "precision": "very-high", | ||
| "severity": "error", | ||
| "short_name": "AdvancedMemoryManagementUsed", | ||
| "tags": [ |
There was a problem hiding this comment.
The tags list only includes the scope tag. Other rule packages typically include at least one classification tag (e.g., correctness, maintainability, reliability) in addition to scope. Consider adding an appropriate classification tag here for better categorization/filtering.
| "tags": [ | |
| "tags": [ | |
| "correctness", |
| #ifndef _GHLIBCPP_UTILITY | ||
| #define _GHLIBCPP_UTILITY | ||
|
|
There was a problem hiding this comment.
tuple.h uses the same include guard macro as utility.h (_GHLIBCPP_UTILITY). This will cause one of the headers to be skipped when both are included, potentially breaking compilation (e.g., <tuple> included before <utility>). Use a unique guard macro for tuple.h (and update the closing #endif comment accordingly).
There was a problem hiding this comment.
In this case copilot is correct, this should be _GHLIBCPP_TUPLE.
|
Holding Copilot's suggestions to not pollute the commit history. |
MichaelRFairhurst
left a comment
MichaelRFairhurst
left a comment
There was a problem hiding this comment.
Nice!
Very thorough and you did an incredible job tracking down / testing all of these APIs!!!!
Mostly just some verbiage suggestions, and a potential class hierarchy tweak.
| // Not in a file called `new`, which is likely to be a copy of the standard library | ||
| // as it is in our tests | ||
| not getFile().getBaseName() = "new" | ||
| not forall(File file | file = this.getADeclarationLocation().getFile() | |
There was a problem hiding this comment.
Based on comments in 28-6-3 (I think?) we should consider making a new base class that isn't for "custom" operator new/delete, but rather, for all operator new/deletes.
This way we can also revert this change back to not getFile().getBaseName() = "new", avoiding potential changes to existing query results.
/**
* ANY new or delete operator, not just custom ones
*/
class OperatorNewOrDelete extends Operator {
OperatorNewOrDelete() {
this.geName().regexpMatch(...)
..
}
}
/**
* ONLY the custom new or delete operators
*/
class CustomOperatorNewOrDelete extends OperatorNewOrDelete { ... }
| @@ -0,0 +1,75 @@ | |||
| /** | |||
There was a problem hiding this comment.
In general, I don't want us to add new files to this directory, as its getting fairly disorganized.
Seems like this could live in cpp/common/src/codingstandards/cpp/standardlibrary/Memory.qll ?
| #ifndef _GHLIBCPP_UTILITY | ||
| #define _GHLIBCPP_UTILITY | ||
|
|
There was a problem hiding this comment.
In this case copilot is correct, this should be _GHLIBCPP_TUPLE.
| string describe() { result = description } | ||
| } | ||
|
|
||
| class NonStandardNewOrNewArrayOperator extends CustomOperatorNewOrDelete { |
There was a problem hiding this comment.
I suggest we implement this more by-the-book. The spec is focused on the signature of the operator new/delete, and I'd say we probably want to follow that here.
Consider something like
// See the above comment on class `OperatorNewOrDelete`
class OperatorNewOrDelete extends Operator { ... } // CustomOperatorNewOrDelete.qll
class AllowedMemoryOperator extends OperatorNewOrDelete {
AllowedMemoryOperator() {
// Check the signature is one of the allowed ones
getParameterCount() = 1 and
(
getParameter(0).getType().resolveTypedefs*() instanceof Size_t and
....
or
...
)
or ...
}
}
class DisallowedMemoryOperator extends OperatorNewOrDelete {
DisallowedMemoryOperator() { not this instanceof AllowedMemoryOperator }
}
| class NonStandardNewOrNewArrayOperator extends CustomOperatorNewOrDelete { | ||
| NonStandardNewOrNewArrayOperator() { | ||
| this.getName() in ["operator new", "operator new[]"] and | ||
| not this instanceof CustomOperatorNew // `CustomOperatorNew` only detects replaceable allocation functions. |
There was a problem hiding this comment.
I see!
Basically, it seems like maybe my comment above should be about a ReplaceableAllocationfunction, rather than using CustomOperatorNew ?
| ::operator delete; // NON_COMPLIANT: implicit address of operator delete | ||
| void (*p3)(void *) = | ||
| &::operator delete[]; // NON_COMPLIANT: address of operator delete[] | ||
| void (*p4)(void *) = ::operator delete[]; // NON_COMPLIANT: implicit address |
There was a problem hiding this comment.
.expected results seem to get out of sync here
| @@ -0,0 +1,88 @@ | |||
| | test.cpp:14:3:14:14 | declaration of operator new | This is a user-provided declaration of `new` / `new[]` / `delete` / `delete[]`. | | |||
There was a problem hiding this comment.
Can we show the operator name here, e.g. Banned declaration of operator new
| */ | ||
| void take_address_of_class_specific_new() { | ||
| void *(*p1)(std::size_t) = | ||
| &C1::operator new; // COMPLIANT: address of class-specific replaceable |
There was a problem hiding this comment.
Are you sure this is compliant? Seems like it should be non compliant to me.
Can we detect this case with exists(operatorNew.getDeclaringType()) ?
| */ | ||
| void take_address_of_class_specific_delete() { | ||
| void (*p1)(void *) = | ||
| &C1::operator delete; // COMPLIANT: address of class-specific |
There was a problem hiding this comment.
Same as above, I believe this should be non-compliant.
| }, | ||
| "queries": [ | ||
| { | ||
| "description": "Dynamically allocated memory must not be managed manually.", |
There was a problem hiding this comment.
Add additional justification here, something like "Dynamic memory management is error prone"/"developers should rely on smart pointers instead where possible", that kind of thing
2 participants
Description
Add packages
Memory5andMemory6, corresponding to Rule 21.6.2 and 21.6.3 respectively.Change request type
.ql,.qll,.qlsor unit tests)Rules with added or modified queries
RULE-21-6-2RULE-21-6-3Release change checklist
A change note (development_handbook.md#change-notes) is required for any pull request which modifies:
If you are only adding new rule queries, a change note is not required.
Author: Is a change note required?
🚨🚨🚨
Reviewer: Confirm that format of shared queries (not the .qll file, the
.ql file that imports it) is valid by running them within VS Code.
Reviewer: Confirm that either a change note is not required or the change note is required and has been added.
Query development review checklist
For PRs that add new queries or modify existing queries, the following checklist should be completed by both the author and reviewer:
Author
As a rule of thumb, predicates specific to the query should take no more than 1 minute, and for simple queries be under 10 seconds. If this is not the case, this should be highlighted and agreed in the code review process.
Reviewer
As a rule of thumb, predicates specific to the query should take no more than 1 minute, and for simple queries be under 10 seconds. If this is not the case, this should be highlighted and agreed in the code review process.